interop: honour needStringCast in the interpreter by aleksisch · Pull Request #3921 · GaijinEntertainment/daScript

aleksisch · 2026-09-01T18:49:31Z

A bind marked needStringCast is handed "" where daslang holds a null string. The AOT backend applies that at the call site (das_string_cast, selected by needStringCast in daslib/aot_cpp.das). The interpreter had no equivalent — it relied on the cast_arg<char *> specializations in ast_typefactory_bind.h, so the conversion reached every bind in a TU that included that header and no bind elsewhere.

Survivable while the call node took the function as a non-type template parameter: it was instantiated once per bound function, in the TU that did the binding, so the choice was at least fixed per bind. Keyed on the signature alone, one node serves every bind of that signature — a game link has 27 objects defining the node for void (*)(char const*), 10 generated dasImgui TUs with the conversion and 17 engine TUs without, and which definition survives depends on object order. ImGui::PushID got a nullptr and crashed in ImHashStr.

This moves the decision to where the bind is known: sv_simulateCall wraps the string arguments of a bind that asked for the conversion, on the same predicate aot_cpp.das uses, and the cast_arg specializations go away. A bind that did not ask still sees null, which is what tests/strings/strings_core_pins holds the string builtins to.

tests/handle_types/string_arg_never_null binds one function twice, with the flag and without, and covers both halves.

daScript deletes that header: the cast_arg specializations it carried move next to the generic cast_arg in simulate/interop.h, and the typeFactory callback fallback next to the primary typeFactory in ast/ast_typedecl.h. The binder no longer emits the include. The wrap had to move because the call node is now keyed on the signature, so one instantiation is shared by binds from many TUs, and only the TUs including this header saw the null-string conversion. Needs GaijinEntertainment/daScript#3921

A bind marked needStringCast is handed "" where daslang holds a null string. The AOT backend applies it at the call site (das_string_cast, picked by needStringCast in daslib/aot_cpp.das). The interpreter had no equivalent: it relied on the cast_arg<char *> specializations that ast_typefactory_bind.h supplies, so the conversion reached every bind in a translation unit which included that header, and no bind elsewhere. That was survivable while SimNode_ExtFuncCall took the function as a non type template parameter: the node was instantiated once per bound function, in the unit that did the binding, so the choice was at least fixed per bind. Keyed on the signature alone, one node serves every bind of that signature. A game link has 27 objects defining the node for void (*)(char const*) - 10 generated dasImgui units which include the header, 17 engine units which do not - and the definition that survives depends on object order. ImGui::PushID got a nullptr and crashed in ImHashStr. Decide it where the bind is known instead: sv_simulateCall wraps the string arguments of a bind which asked for the conversion, on the same predicate aot_cpp.das uses, and the cast_arg specializations go away. A bind which did not ask still sees null, which is what tests/strings/strings_core_pins holds the string builtins to. tests/handle_types/string_arg_never_null binds one function twice, with the flag and without, and covers both halves.

borisbat

way too slow.

borisbat · 2026-09-01T23:03:39Z

+        SimNode_StringArgNotNull ( const LineInfo & a, SimNode * se ) : SimNode(a), subexpr(se) {}
+        __forceinline char * compute ( Context & context ) {
+            DAS_PROFILE_NODE
+            char * res = subexpr->evalPtr(context);


this is going to be slower than original implementation, which uses cast<>
cast gets inlined on C++ side, this creates interpreter node - which is a lot slower
even blanket cast_arg<char *> would be faster

Copilot

🔵 Needs a closer look

It changes core interop null-string semantics for every string bind across the engine and removes a public installed header, a high-impact cross-cutting change that warrants final human review despite being well-tested.

Pull request overview

This PR moves the "null daScript string → empty C string" conversion for interop binds from a header-wide, per-TU mechanism into the interpreter's simulate path, keyed on the bind's needStringCast flag — matching how the AOT (das_string_cast in aot_cpp.das) and JIT (llvm_jit.das) backends already behave. Previously the conversion lived in cast_arg<char*>/cast_arg<const char*> specializations inside ast_typefactory_bind.h, which applied to every bind in any TU that happened to include that header and to none elsewhere — an unstable, link-order-dependent behavior (the motivating ImGui::PushID crash). The header is deleted and its still-needed pieces relocated.

Changes:

Add SimNode_StringArgNotNull and wrap string arguments of needStringCast binds in sv_simulateCall, on the same predicate the AOT/JIT tiers use.
Delete ast_typefactory_bind.h, relocating typeFactory<ResT(*)(Args...)> into ast_typedecl.h and cast_arg<das::string> into interop.h; drop its include from ~90 module/tutorial/example TUs, the cbind generator, and the install list.
Add a UnitTest bind pair (test_string_arg_length / _cast) and tests/handle_types/string_arg_never_null.das covering both halves.

File summaries

File	Description
`include/daScript/simulate/simulate_nodes.h`	New `SimNode_StringArgNotNull` node (null → `""`).
`src/simulate/simulate_visit.cpp`	Visitor plumbing for the new node (matches `KeepAlive`).
`src/ast/ast_simulate.cpp`	Wraps `needStringCast` string args in `sv_simulateCall`.
`include/daScript/simulate/interop.h`	Adds relocated `cast_arg<das::string>`.
`include/daScript/ast/ast_typedecl.h`	Adds relocated `typeFactory<ResT(*)(Args...)>`.
`include/daScript/ast/ast_typefactory_bind.h`	Deleted (specializations relocated / dropped).
`modules/dasUnitTest/test_handles.cpp`	Adds `testStringArgLength` bound with and without `needStringCast`.
`tests/handle_types/string_arg_never_null.das`	New test covering cast, non-cast, empty, null, non-empty.
`CMakeLists.txt`	Removes deleted header from release AST include list.
`modules/dasClangBind/cbind/cbind_boost.das`	Stops emitting the deleted header in generated binds.
~90 module/tutorial/example `.cpp` files	Drop the now-unused header include.

One minor, non-blocking note (not filable — the file isn't in the diff): tests/README.md asks that every .das under tests/ be listed, but its handle_types/ section is already stale (missing several files); consider adding string_arg_never_null.das there.

Review details

Files reviewed: 112/112 changed files
Comments generated: 0
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

aleksisch closed this Sep 1, 2026

aleksisch reopened this Sep 1, 2026

aleksisch force-pushed the fix/cast-arg-string-instantiation branch from e2dcbd5 to 39ebd8e Compare September 1, 2026 20:10

aleksisch changed the title ~~interop: keep the null string wrap for every call node instantiation~~ interop: keep interop traits next to the templates they specialize Sep 1, 2026

aleksisch force-pushed the fix/cast-arg-string-instantiation branch 2 times, most recently from 95956eb to c93f036 Compare September 1, 2026 20:29

This was referenced Sep 1, 2026

regenerate: drop the ast_typefactory_bind.h include borisbat/dasImguiImplot#30

Merged

regenerate: drop the ast_typefactory_bind.h include borisbat/dasImguiNodeEditor#46

Merged

aleksisch changed the title ~~interop: keep interop traits next to the templates they specialize~~ interop: honour needStringCast in the interpreter Sep 1, 2026

aleksisch force-pushed the fix/cast-arg-string-instantiation branch from c93f036 to cdc7830 Compare September 1, 2026 22:03

aleksisch force-pushed the fix/cast-arg-string-instantiation branch from cdc7830 to b155f3f Compare September 1, 2026 22:09

aleksisch requested a review from borisbat September 1, 2026 22:31

borisbat requested a balanced review from Copilot September 1, 2026 22:59

Copilot started reviewing on behalf of borisbat September 1, 2026 22:59 View session

borisbat requested changes Sep 1, 2026

View reviewed changes

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Navigation Menu

Uh oh!

interop: honour needStringCast in the interpreter - #3921

interop: honour needStringCast in the interpreter#3921
aleksisch wants to merge 1 commit into
GaijinEntertainment:masterfrom
aleksisch:fix/cast-arg-string-instantiation

aleksisch commented Sep 1, 2026 •

edited

Loading

Uh oh!

borisbat left a comment

Uh oh!

borisbat Sep 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

aleksisch commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borisbat left a comment

Choose a reason for hiding this comment

Uh oh!

borisbat Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aleksisch commented Sep 1, 2026 •

edited

Loading